Allow configuring (opt-in) IMC async handler #8311

pierDipi · 2024-11-07T08:25:53Z

We switched to use the sync handler by default, however, it was
reported that in some cases, this is not wanted as it slows down
the source event senders since it needs to wait for all subscribers
to receive events.

While sync handler is the best default behavior since reduces lost events in
InMemoryChannel, we want to allow configuring this behavior, while
documenting the downsides (follow up to docs repo)

Proposed Changes

Allow configuring (opt-in) IMC async handler
Propagate annotations and labels from MTChannelBasedBroker to underlying channel

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

Allow configuring (opt-in) IMC async handler

Docs

codecov · 2024-11-07T08:41:54Z

Codecov Report

Attention: Patch coverage is 91.17647% with 3 lines in your changes missing coverage. Please review.

Project coverage is 64.62%. Comparing base (6b4ff7f) to head (b1e6f3d).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/reconciler/testing/v1/broker.go	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8311      +/-   ##
==========================================
+ Coverage   64.58%   64.62%   +0.03%     
==========================================
  Files         373      373              
  Lines       22619    22649      +30     
==========================================
+ Hits        14609    14636      +27     
- Misses       7244     7246       +2     
- Partials      766      767       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

matzew · 2024-11-07T10:12:07Z

pkg/channel/fanout/fanout_event_handler.go

@@ -60,8 +60,11 @@ type Subscription struct {
 // Config for a fanout.EventHandler.
 type Config struct {
 	Subscriptions []Subscription `json:"subscriptions"`
-	// Deprecated: AsyncHandler controls whether the Subscriptions are called synchronous or asynchronously.


should we release note that this is not deprecated anymore?

This is only at the code level / library and we usually don't release note it since pretty much all users don't care

matzew · 2024-11-07T10:13:32Z

pkg/reconciler/inmemorychannel/dispatcher/inmemorychannel.go

 	return &multichannelfanout.ChannelConfig{
 		Namespace: imc.Namespace,
 		Name:      imc.Name,
 		HostName:  imc.Status.Address.URL.Host,
 		Path:      fmt.Sprintf("%s/%s", imc.Namespace, imc.Name),
 		FanoutConfig: fanout.Config{
-			AsyncHandler:  false,


seems like a good idea to pass in the bool via configuration/annotation in case folks want to have it "async", and do not care about the impact.

matzew · 2024-11-07T10:14:15Z

While sync handler is the best default behavior since reduces lost events in
InMemoryChannel, we want to allow configuring this behavior, while
documenting the downsides (follow up to docs repo)

Seems like a good idea to document this. for the next upstream release.

/lgtm

matzew · 2024-11-07T16:41:46Z

/test reconciler-tests

matzew · 2024-11-08T07:40:51Z

/test reconciler-tests

matzew · 2024-11-08T08:13:44Z

=== FAIL: test/rekt TestBrokerPropagatesMetadata/Broker_PreferHeader_Check/Setup/install_broker (0.12s)
    broker.go:151: failed to create resource Broker in version "v1" cannot be handled as a Broker: json: cannot unmarshal bool into Go struct field ObjectMeta.annotations of type string - Resource:
        apiVersion: eventing.knative.dev/v1
        kind: Broker
        metadata:
          annotations:
            eventing.knative.dev/broker.class: MTChannelBasedBroker
            messaging.knative.dev/async-handler: false
          name: broker-bnjzunfv
          namespace: test-ckrgeeak
        spec: null
        
=== FAIL: test/rekt TestChannelAsyncHandler/AsyncHandlerUpdate/Requirement/update_channel_async_handler (0.40s)
    channel_impl.go:92: failed to update resource admission webhook "validation.inmemorychannel.eventing.knative.dev" denied the request: validation failed: Channel.Spec.Subscribers changed by user [email protected] which was not the system:serviceaccount:knative-eventing:eventing-controller service account: spec.subscribers
        {[]v1.SubscriberSpec}:
        	-: "[{Name:0xc0009a5ea0 UID:828704ea-0f50-48ab-bcd7-9b9cacc19796 Generation:1 SubscriberURI:http://sink-dwarkbpn.test-wcdvjxod.svc.cluster.local/ SubscriberCACerts: SubscriberAudience: ReplyURI: ReplyCACerts: ReplyAudience: Delivery: Auth:}]"
        	+: "[]"
         - Resource:
        apiVersion: messaging.knative.dev/v1
        kind: InMemoryChannel
        metadata:
          name: channel-nnnoytbn
          namespace: test-wcdvjxod
        spec: null

matzew · 2024-11-11T12:58:32Z

@pierDipi looks like some unmarshal error for the boolean value

matzew · 2024-11-11T14:55:30Z

pkg/reconciler/broker/broker.go

+	}
+	for k, v := range metadata.GetAnnotations() {
+		if strings.HasPrefix(k, messagingv1.SchemeGroupVersion.Group) {
+			channelAnnotations[k] = v


is this always a string?

matzew · 2024-11-11T16:50:51Z

test/rekt/resources/broker/broker.yaml

@@ -24,7 +24,7 @@ metadata:
  {{ end }}
  {{ if .annotations }}
    {{ range $key, $value := .annotations }}
-    {{ $key }}: {{ $value }}
+    {{ $key }}: "{{ $value }}"


for strings, using explicit ""... made the TestBrokerPropagatesMetadata work

(works on my machine 😅 kinda fix)

matzew · 2024-11-11T16:52:18Z

=== FAIL: test/rekt TestChannelAsyncHandler/AsyncHandlerUpdate/Requirement/update_channel_async_handler (0.40s)
    channel_impl.go:92: failed to update resource admission webhook "validation.inmemorychannel.eventing.knative.dev" denied the request: validation failed: Channel.Spec.Subscribers changed by user [email protected] which was not the system:serviceaccount:knative-eventing:eventing-controller service account: spec.subscribers
        {[]v1.SubscriberSpec}:
        	-: "[{Name:0xc0009a5ea0 UID:828704ea-0f50-48ab-bcd7-9b9cacc19796 Generation:1 SubscriberURI:http://sink-dwarkbpn.test-wcdvjxod.svc.cluster.local/ SubscriberCACerts: SubscriberAudience: ReplyURI: ReplyCACerts: ReplyAudience: Delivery: Auth:}]"
        	+: "[]"
         - Resource:
        apiVersion: messaging.knative.dev/v1
        kind: InMemoryChannel
        metadata:
          name: channel-nnnoytbn
          namespace: test-wcdvjxod
        spec: null

is because of this: https://github.com/knative/eventing/blob/main/pkg/apis/messaging/v1/in_memory_channel_validation.go#L92

CC @pierDipi @Cali0707

matzew · 2024-11-12T06:05:59Z

/retest

matzew · 2024-11-12T15:19:07Z

test/rekt/resources/channel_impl/channel_impl.yaml

@@ -17,6 +17,12 @@ kind: {{ .kind }}
 metadata:
  name: {{ .name }}
  namespace: {{ .namespace }}
+  annotations:


when looking at the reconciler tests, I never saw the messaging.knative.dev/async-handler annotation, hence adding this

matzew · 2024-11-12T15:21:10Z

test/rekt/features/channel/features.go

@@ -191,12 +193,28 @@ func AsyncHandlerUpdate(createSubscriberFn func(ref *duckv1.KReference, uri stri
 	f.Setup("channel is ready", channel_impl.IsReady(name))
 	f.Setup("subscription is ready", subscription.IsReady(sub))

-	f.Requirement("update channel async handler", channel_impl.Install(name, channel_impl.WithAnnotations(map[string]interface{}{


looking at the error again:

channel_impl.go:92: failed to update resource admission webhook "validation.inmemorychannel.eventing.knative.dev" denied the request: validation failed: Channel.Spec.Subscribers changed by user [email protected] which was not the system:serviceaccount:knative-eventing:eventing-controller service account: spec.subscribers {[]v1.SubscriberSpec}: -: "[{Name:0xc0009a5ea0 UID:828704ea-0f50-48ab-bcd7-9b9cacc19796 Generation:1 SubscriberURI:http://sink-dwarkbpn.test-wcdvjxod.svc.cluster.local/ SubscriberCACerts: SubscriberAudience: ReplyURI: ReplyCACerts: ReplyAudience: Delivery: Auth:}]" +: "[]"

I saw that the subscribers are "replaced" w/ nothing (+:"[]"), which lead me to the fact that we here just re-install the channel_impl, based from its manifest yaml file, which lacks the actual subscribers

We switched to use the sync handler by default, however, it was reported that in some cases, this is not wanted as it slows down the source event senders since it needs to wait for all subscribers to receive events. While this is the best default behavior since reduces lost events in InMemoryChannel, we want to allow configuring this behavior, while documenting the downsides (follow up to docs repo) Signed-off-by: Pierangelo Di Pilato <[email protected]>

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Signed-off-by: Matthias Wessendorf <[email protected]>

matzew · 2024-11-12T15:44:21Z

@pierDipi I've update the PR, for the async-handler update. Mainly changing the test. See comments above

matzew · 2024-11-12T16:26:08Z

/test unit-tests

…s. Otherwise we loose the channel's Spec.Subscribers Signed-off-by: Matthias Wessendorf <[email protected]>

pierDipi

Thanks @matzew !

LGTM (I can't LGTM my own PR but the new changes look good)

matzew

/lgtm
/approve

knative-prow · 2024-11-18T09:24:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [matzew,pierDipi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

matzew · 2024-11-18T13:46:35Z

/retest Sent from Gmail Mobile

…

On Mon 18. Nov 2024 at 11:13, knative-prow[bot] ***@***.***> wrote: @pierDipi <https://github.com/pierDipi>: The following test *failed*, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests: Test name Commit Details Required Rerun command reconciler-tests_eventing_main b1e6f3d <b1e6f3d> link <https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_eventing/8311/reconciler-tests_eventing_main/1858441207496052736> unknown /test reconciler-tests Your PR dashboard <https://prow.knative.dev/pr>. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow <https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — Reply to this email directly, view it on GitHub <#8311 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGPTWCRN4Z6ZQ6NQROI332BG4RDAVCNFSM6AAAAABRKT5V6GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOBSGUZTEMBZHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/test-and-release Test infrastructure, tests or release labels Nov 7, 2024

knative-prow bot requested review from aslom and evankanderson November 7, 2024 08:25

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2024

pierDipi force-pushed the allow-configuring-imc-async-handler branch from 83f210b to 055fae7 Compare November 7, 2024 08:54

matzew reviewed Nov 7, 2024

View reviewed changes

knative-prow bot assigned matzew Nov 7, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2024

matzew reviewed Nov 11, 2024

View reviewed changes

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 11, 2024

matzew reviewed Nov 11, 2024

View reviewed changes

matzew force-pushed the allow-configuring-imc-async-handler branch from 0feaf0b to 1389906 Compare November 12, 2024 07:38

matzew reviewed Nov 12, 2024

View reviewed changes

pierDipi added 3 commits November 12, 2024 16:42

Propagate annotations and labels to channel

cd2b5e9

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Add E2E tests

c200cff

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi and others added 5 commits November 12, 2024 16:42

Use constant in tests

66375bb

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Only propagates messaging.knative.dev annotations

b187b70

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Add unit test

32bf295

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Make annotation val explicit string.......

acc4557

Signed-off-by: Matthias Wessendorf <[email protected]>

channel impl did not support setting annotations before........

35def44

Signed-off-by: Matthias Wessendorf <[email protected]>

matzew force-pushed the allow-configuring-imc-async-handler branch from c180ebe to 859dfff Compare November 12, 2024 15:42

Instead of re-installing the channel_impl, lets update the annotation…

b1e6f3d

…s. Otherwise we loose the channel's Spec.Subscribers Signed-off-by: Matthias Wessendorf <[email protected]>

matzew force-pushed the allow-configuring-imc-async-handler branch from 859dfff to b1e6f3d Compare November 12, 2024 16:44

pierDipi commented Nov 18, 2024

View reviewed changes

pierDipi changed the title ~~[WIP] Allow configuring (opt-in) IMC async handler~~ Allow configuring (opt-in) IMC async handler Nov 18, 2024

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2024

matzew approved these changes Nov 18, 2024

View reviewed changes

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2024

knative-prow bot merged commit 8fed0be into knative:main Nov 18, 2024
35 of 36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring (opt-in) IMC async handler #8311

Allow configuring (opt-in) IMC async handler #8311

pierDipi commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

matzew Nov 7, 2024

pierDipi Nov 18, 2024

matzew Nov 7, 2024

matzew commented Nov 7, 2024

matzew commented Nov 7, 2024

matzew commented Nov 8, 2024

matzew commented Nov 8, 2024

matzew commented Nov 11, 2024

matzew Nov 11, 2024

matzew Nov 11, 2024

matzew Nov 11, 2024

matzew commented Nov 11, 2024

matzew commented Nov 12, 2024

matzew Nov 12, 2024

matzew Nov 12, 2024

matzew commented Nov 12, 2024

matzew commented Nov 12, 2024

pierDipi left a comment

matzew left a comment

knative-prow bot commented Nov 18, 2024

matzew commented Nov 18, 2024 via email

Allow configuring (opt-in) IMC async handler #8311

Allow configuring (opt-in) IMC async handler #8311

Conversation

pierDipi commented Nov 7, 2024 • edited Loading

Proposed Changes

Pre-review Checklist

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matzew commented Nov 7, 2024

matzew commented Nov 7, 2024

matzew commented Nov 8, 2024

matzew commented Nov 8, 2024

matzew commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matzew commented Nov 11, 2024

matzew commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matzew commented Nov 12, 2024

matzew commented Nov 12, 2024

pierDipi left a comment

Choose a reason for hiding this comment

matzew left a comment

Choose a reason for hiding this comment

knative-prow bot commented Nov 18, 2024

matzew commented Nov 18, 2024 via email

pierDipi commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading